JSX V4 part 1 (to be continued). #547

mununki · 2022-06-15T07:33:11Z

This PR rewrites the JSX PPX which is aligned to #521.
Here's the Spec at the time of writing this. But please check if the spec has changed as part of this PR.

A further plans

tests/ppx/react/expected/commentAtTop.res.txt

cristianoc

A small comment on style: this looks a little strange:

let makePropsTypeParamsSig namedTypeList =
  namedTypeList
  |> List.filter_map (fun (isOptional, label, _, interiorType) ->
         if label = "key" || label = "ref" then None
         else if isOptional then Some (extractOptionalCoreType interiorType)
         else Some interiorType)

as it removes an @optional from the type. But that annotation was added by the ppx itself.

It might indicate that the logic for doing this should probably be further back in the caller, not in this function. So that the @optional attribute is never added in the first place. Otherwise, it could be easy to introduce bugs via refactoring without realizing it.

cristianoc

Also this is pre-existing, but now that the code is formatted it really jumps to the eye.

Wrapping things in let jsxMapper () = indents everything, and groups a bunch of functions that don't really need to be grouped. How about just remove it and bring everything to the top level.

cristianoc · 2022-06-17T00:22:53Z

Also this is pre-existing, but now that the code is formatted it really jumps to the eye.

Wrapping things in let jsxMapper () = indents everything, and groups a bunch of functions that don't really need to be grouped. How about just remove it and bring everything to the top level.

At that point, this global state:

let nestedModules = ref []

should be created in rewrite_implementation and rewrite_signature and passed around to the functions that need it.

mununki · 2022-06-17T04:23:07Z

A small comment on style: this looks a little strange:
let makePropsTypeParamsSig namedTypeList =
  namedTypeList
  |> List.filter_map (fun (isOptional, label, _, interiorType) ->
         if label = "key" || label = "ref" then None
         else if isOptional then Some (extractOptionalCoreType interiorType)
         else Some interiorType)
as it removes an @optional from the type. But that annotation was added by the ppx itself.

It might indicate that the logic for doing this should probably be further back in the caller, not in this function. So that the @optional attribute is never added in the first place. Otherwise, it could be easy to introduce bugs via refactoring without realizing it.

Absolutely, I totally forgot this part of generating sig, this strange code was written before you introduced the @optional syntax. I'll fix it.

mununki · 2022-06-17T04:29:10Z

Also this is pre-existing, but now that the code is formatted it really jumps to the eye.

Wrapping things in let jsxMapper () = indents everything, and groups a bunch of functions that don't really need to be grouped. How about just remove it and bring everything to the top level.

It sounds good to me!

cli/reactjs_jsx_ppx_v3.ml

cristianoc · 2022-06-18T06:37:57Z

Probably best to rebase on master (see #565) sooner rather than later.
To avoid piling up merge conflicts.

mununki · 2022-06-19T03:06:11Z

What is the core team's plan for ppx v4 to support the new jsx transform? I found some feedbacks in forum that they want it, but there is a review not to support the new jsx transform. rescript-lang/rescript-react#1 (comment)
I think it is an easy task to make the ppx v4 support the new jsx transform, but I have questions about it.

Does the ppx v4 need to support the new jsx transform? or support them both?
If the ppx v4 needs to support them both with user's preference, what is the interface to get a configuration value from the compiler?

cristianoc · 2022-06-19T05:08:41Z

What is the core team's plan for ppx v4 to support the new jsx transform? I found some feedbacks in forum that they want it, but there is a review not to support the new jsx transform. rescript-lang/rescript-react#1 (comment) I think it is an easy task to make the ppx v4 support the new jsx transform, but I have questions about it.

Does the ppx v4 need to support the new jsx transform? or support them both?

If the ppx v4 needs to support them both with user's preference, what is the interface to get a configuration value from the compiler?

One constraint is that the initial release will require no changes to rescript-react.
Does this constraint affect the ability to support the new jsx transform?

mununki · 2022-06-19T05:16:22Z

One constraint is that the initial release will require no changes to rescript-react.
Does this constraint affect the ability to support the new jsx transform?

The APIs for the new jsx transform are bound already here, but those are having attribute @deprecated. I think ppx v4 can use it without changes in rescript-react, but users will encounter countless warnings I guess.

cristianoc · 2022-06-19T05:19:13Z

One constraint is that the initial release will require no changes to rescript-react.
Does this constraint affect the ability to support the new jsx transform?

The APIs for the new jsx transform are bound already https://github.com/rescript-lang/rescript-react/blob/ef325291733c8d42a53d87e8317371cbb6381011/src/React.res#L28, but those are having attribute @deprecated. I think ppx v4 can use it without changes in rescript-react, but users will encounter countless warnings I guess.

The PPX can issue code that silences deprecation warnings. Just like it can do for unused variables.
Another question is whether using the new jsx transform is a breaking change for existing projects?

mununki · 2022-06-19T05:36:17Z

Another question is whether using the new jsx transform is a breaking change for existing projects?

Sure, the first case that comes to mind is the project which has the dependencies under React v17. But I can't claim other cases for now. Maybe we can ask it in the forum.

cristianoc · 2022-06-19T05:37:53Z

Definitely something for the forum. Really need to know whom this will break. As that would make it necessary to have it configurable.
If configurable, probably in bsconfig.json.

mununki · 2022-06-19T05:45:26Z

If configurable, probably in bsconfig.json.

Yes, it is workable as one of configurations in bsconfig.json.

"reason": {
  "react-jsx": 4,
  "react-runtime": "automatic"  // or "classic" like the Babel configuration
}

mununki · 2022-06-19T08:19:36Z

I'm trying to figure out how to access the bsconfig value, but I think I'm not on right track. Can you give me a clue?
AFAIK, all syntax modules are copied to /jscomp/napkin in compiler for building, then napkin.cmx is linked to bsc.exe. But, the parser for the bsconfig.json and config values are accessed in rescript.exe only. Is it possible for ppx module to access the config value? I guess it is only possible backward. I mean bsc.exe is executed with args, so that the additional cli arg is needed to be executed by rescript.exe.

cristianoc · 2022-06-19T09:28:09Z

I think that's right. bsc needs to be as fast as possible so it gets all the options passed to

mununki · 2022-06-19T10:59:08Z

It makes sense that the separate binary seems an architectural choice for performance.

mununki · 2022-06-19T23:56:44Z

I figured out and tested adding a new cli arg between rescript.exe and bsc.exe successfully. Now I can get a configuration value from bsconfig.json to syntax. I have a question and ask your thought about user interface and API between compiler and syntax.

bsconfig.json

"reason": {
  "react-jsx": 3 // or 4
  "react-runtime": "classic" // or "automatic"
}

cli args

-bs-jsx [3 or 4]
-bs-react-runtime [classic or automatic]

In this regard, If we put the ppx v3 and v4 both, then we can make users migrate their projects gracefully. What do you think?

cristianoc · 2022-06-20T00:22:43Z

I figured out and tested adding a new cli arg between rescript.exe and bsc.exe successfully. Now I can get a configuration value from bsconfig.json to syntax. I have a question and ask your thought about user interface and API between compiler and syntax.

bsconfig.json
"reason": {
  "react-jsx": 3 // or 4
  "react-runtime": "classic" // or "automatic"
}
cli args
-bs-jsx [3 or 4]
-bs-react-runtime [classic or automatic]
In this regard, If we put the ppx v3 and v4 both, then we can make users migrate their projects gracefully. What do you think?

Suggestion: "reason" called "react" instead.
Then just "jsx" and "runtime".
There will be the schema to update when this is final, but there's time for that.

This could be a good time to begin thinking about migration. Having a version flag is useful, though not enough, to support migration. The flag is what's going to be used at the end of a gradual migration. What's left is supporting intermediate migration steps when one only migrates e.g. one component, or one dependent project.

cristianoc · 2022-06-20T00:24:46Z

Something to check, once the ppx version flag is in place: what happens with dependencies? I guess they will be recompiled with whatever setting is used in the current project. But one needs to verify that this is true.

* jsx version check is conducted in compiler config side

cristianoc · 2022-06-24T19:00:38Z

How would you like to proceed.
By separating into v3 and v4 it's possible to review and merge this in soon, and keep on refining in a follow-on PR.

mununki · 2022-06-25T02:48:51Z

Sure, why not. I prefer keeping the open time of PR short either. Maybe, we can ship the ppx v4 as an experimental feature with V10 release, it allows us to get feedback and chances to refine the v4 and rescript-react.
As a side comment, I'd like to ask the core team for any chance to update the rescript-react for the official release ppx v4. I think there are missing bindings for the new jsx transform and it is outdated such asReasonReact.fragment.

cristianoc · 2022-06-25T02:52:53Z

How about putting bindings for the new jsx transform directly inside the compiler? Under the Js.* namespace.
As from what I understand is not bound to react necessarily.

cristianoc · 2022-06-25T02:55:49Z

cli/res_cli.ml

+         Default: none" );
+      ( "-jsx-runtime",
+        Arg.String (fun txt -> jsxRuntime := txt),
+        "Specify the jsx runtime for React, classic or automatic. Default: \


This option only affects V4 right?

Yes, it affects V4 only.

cristianoc · 2022-06-25T02:59:13Z

src/res_core.ml

@@ -2604,6 +2605,28 @@ and parseJsxProp p =
          if optional then Asttypes.Optional name else Asttypes.Labelled name
        in
        Some (label, attrExpr))
+  (* {...props} *)
+  | Lbrace -> (


What happens when this is parsed while V3 is active?
Should this be gated on version or is it OK like this?

It will be OK with V3. V9 will give us a parsing error, but V10 will parse the JSX application as below, but it will be a type error anyway. Because the definition doesn't have a spreadProps as a prop. IMO, it doesn't make anything worse.

// lowercase ReactDOMRe.createDOMElementVariadic( "div", ~props=ReactDOMRe.domProps(~spreadProps=foo, ()), [], ) // uppercase React.createElement(Foo.make, Foo.makeProps(~spreadProps=foo, ())),

cristianoc

Some minor comments.
Just checking that nothing is broken in V3 support at the moment.
Not really reviewing V4 fully.

cristianoc · 2022-06-25T03:02:45Z

I think what's out there, modulo a couple of pending questions, can be merged. And work on V4, and review on it, can continue separately.
So that one does not need to keep on rebasing this PR when master changes.
One obvious thing to review is the use of locations in V4, and what happens with editor integration. But it's OK to do that later.

mununki · 2022-06-25T08:13:33Z

How about putting bindings for the new jsx transform directly inside the compiler? Under the Js.* namespace. As from what I understand is not bound to react necessarily.

I can put the new jsx binding into Js.*, but can it make an unnecessary dependency of the compiler with react?

cristianoc · 2022-06-25T08:21:14Z

How about putting bindings for the new jsx transform directly inside the compiler? Under the Js.* namespace. As from what I understand is not bound to react necessarily.

I can put the new jsx binding into Js.*, but can it make an unnecessary dependency of the compiler with react?
There's an entire ppx in the compiler. A couple of bindings don't seem like a big deal.
But, would like to see what they look like / how they are used to know better.

mununki · 2022-06-25T08:21:41Z

How about putting bindings for the new jsx transform directly inside the compiler? Under the Js.* namespace. As from what I understand is not bound to react necessarily.

I can put the new jsx binding into Js.*, but can it make an unnecessary dependency of the compiler with react?

How about adding some missing binding into rescript-react, and make additional type such as type domProps2, type props2 for V4 in ReactDOM.res with @optional attribute. I think it will not break anything.

cristianoc · 2022-06-25T08:21:41Z

I think this can be merged right now, then continued.

mununki · 2022-06-25T08:24:13Z

Thanks for merging, I'm gonna make the new PR part 2.

This was referenced Jun 15, 2022

JSX V4 WIP #517

Closed

Update React modules in jscomp/test rescript-lang/rescript#5438

Closed

cristianoc added this to the v10.1 milestone Jun 15, 2022

cristianoc reviewed Jun 15, 2022

View reviewed changes

tests/ppx/react/expected/commentAtTop.res.txt Outdated Show resolved Hide resolved

mununki force-pushed the react-ppx-v4 branch from f8e7273 to 3e54838 Compare June 16, 2022 17:16

cristianoc reviewed Jun 17, 2022

View reviewed changes

mununki closed this Jun 17, 2022

mununki reopened this Jun 17, 2022

mununki commented Jun 17, 2022

View reviewed changes

cli/reactjs_jsx_ppx_v3.ml Outdated Show resolved Hide resolved

cristianoc mentioned this pull request Jun 17, 2022

WIP: React JSX PPX v4 #235

Closed

4 tasks

mununki commented Jun 17, 2022

View reviewed changes

cli/reactjs_jsx_ppx_v3.ml Outdated Show resolved Hide resolved

mununki force-pushed the react-ppx-v4 branch from 9c3cb50 to d2a11ef Compare June 18, 2022 23:54

mununki added 13 commits June 25, 2022 01:55

make react component name capitalized

139e23a

test: add test files

c975041

fix ppx for forwardRef

5a1dc66

remove ref from props except forwardRef

9a2ddb0

add test for forwardRef

5aa2382

format

3c21396

remove uneccessary adding @optional attr from extracting props type

2e4025d

indent by relocating every fn from jsxMapper

5216cce

remove unused jsxVersion

34f9cf8

* jsx version check is conducted in compiler config side

add new line

256eaac

update jsx ppx comment

007b17b

rename v3 -> v4, restore v3, add cli args

0bacbe3

add tests for v3 and v4 respectively

0337629

mununki force-pushed the react-ppx-v4 branch from 23a6ca1 to 0337629 Compare June 24, 2022 16:55

cristianoc reviewed Jun 25, 2022

View reviewed changes

cristianoc merged commit 5dedb5b into rescript-lang:master Jun 25, 2022

cristianoc changed the title ~~JSX V4~~ JSX V4 part 1 (to be continued). Jun 25, 2022

mununki deleted the react-ppx-v4 branch June 25, 2022 08:23

mununki mentioned this pull request Jun 29, 2022

JSX PPX V4 part2 #586

Closed

13 tasks

JSX V4 part 1 (to be continued). #547

JSX V4 part 1 (to be continued). #547

Uh oh!

Conversation

mununki commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

cristianoc commented Jun 17, 2022

Uh oh!

mununki commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mununki commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cristianoc commented Jun 18, 2022

Uh oh!

mununki commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc commented Jun 19, 2022

Uh oh!

mununki commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc commented Jun 19, 2022

Uh oh!

mununki commented Jun 19, 2022

Uh oh!

cristianoc commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mununki commented Jun 19, 2022

Uh oh!

mununki commented Jun 19, 2022

Uh oh!

cristianoc commented Jun 19, 2022

Uh oh!

mununki commented Jun 19, 2022

Uh oh!

mununki commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bsconfig.json

cli args

Uh oh!

cristianoc commented Jun 20, 2022

bsconfig.json

cli args

Uh oh!

cristianoc commented Jun 20, 2022

Uh oh!

cristianoc commented Jun 24, 2022

Uh oh!

mununki commented Jun 25, 2022

Uh oh!

cristianoc commented Jun 25, 2022

Uh oh!

cristianoc Jun 25, 2022

Choose a reason for hiding this comment

Uh oh!

mununki Jun 25, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc Jun 25, 2022

Choose a reason for hiding this comment

Uh oh!

mununki Jun 25, 2022

Choose a reason for hiding this comment

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

mununki commented Jun 15, 2022 •

edited

Loading

mununki commented Jun 17, 2022 •

edited

Loading

mununki commented Jun 17, 2022 •

edited

Loading

mununki commented Jun 19, 2022 •

edited

Loading

mununki commented Jun 19, 2022 •

edited

Loading

cristianoc commented Jun 19, 2022 •

edited

Loading

mununki commented Jun 19, 2022 •

edited

Loading

cristianoc commented Jun 25, 2022 •

edited

Loading

mununki commented Jun 25, 2022 •

edited

Loading